Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

I'm not sure I understand how the current implementation of the theme system works. It says here that it works by changing the href of the light.css <link> tag. But then, what's the point of including <link> tags for every other theme (especially if they stay disabled).

I'd like to know so I can properly implement a fix to support JavaScript disabled browsers to use the system theme :)

@Cldfire might know ^

The disabled attribute is used to prevent the style to be applied, however they still are loaded by the browser in case you want to switch theme. Which means that when you arrive on a page, you won't have the light theme then the selected theme displayed (it would look like a weird flash haha).

I tried getting rid of the disabled attribute in #75063, but unfortunately I wasn't able to figure out how to get rid of it in a way that's supported everywhere and doesn't result in content flashing on page load.

Usage of the disabled attribute was first introduced in #71237; prior to that PR all themes were enabled at the same time and therefore had to have exactly the same CSS rules (otherwise they'd conflict with one another).

What is the forward/backwards compatibility of this? All docs hosted on docs.rs are using a shared localStorage but the javascript from when they were built, so will this behave well when you change the settings in docs with this new JS then open docs with the old JS?

I didn't change the way localStorage is used at all. I only added new settings, and the default theme behavior (system theme) will be kept. So it's 100% backwards and forward compatible!

It actually even improves backwards: when the system theme is set, it's saved to localStorage so that when the user visits a page using the old JS, it will stay on the theme of the user's system.

(This also looks relatively easy for docs.rs to hijack to configure its own theme (when we get support), other than the lack of self-storage-events to make it easy for us to detect when to check the value).

@Nemo157 Yes, I said a while ago that I needed to do it but still haven't found time to do so... I'll try to get back to it once this PR is merged.

@nasso Looks all good to me, thanks! Just remains to replace single quotes with double quotes and then it's good to go.

Actually I think I should make sure it works properly with JavaScript disabled before merging. Currently it doesn't work, because the logic is in storage.js. One way i see to fix it would be to add a default-theme.css stylesheet that would look like this:

@import "{static_root_path}light{suffix}.css" (prefers-color-scheme: light);
@import "{static_root_path}dark{suffix}.css" (prefers-color-scheme: dark);

The problem with this is that the styles might override each other in bad ways (since there's already the javascript controlled <link>). My idea was to make that <link> default to default-theme.css instead of light.css, and let JavaScript override it if necessary. I don't know if this will cause content flashing though, I'll have to test it first if you don't see any other issue it might cause.

Actually I think I should make sure it works properly with JavaScript disabled before merging.

By default it's the light theme that gets displayed, so if you get the same result, it's working the same as currently! But if you want to extend it (which would be nice!), please do so but in another PR so we can get this one merged.

Actually the old behavior was using the system theme by default! You can try it by opening the standard library documentation in a private window (so that localStorage is empty). According to your system's theme, it will either use the "dark" or the "light" theme. Only thing is that you have to reload the page to update the theme when you change it in your operating system settings.

Hmmmm... Makes sense in a way I guess?

Yeah, because the old method used getSystemTheme as the default value if rustdoc-theme wasn't set in localStorage. The "light" theme was only the second fallback. See here in storage.js.

So should I add support for JavaScript disabled browsers in another PR or this one to keep backwards compatibility with the old behavior?

According to your system's theme, it will either use the "dark" or the "light" theme.

Isn't that the intended behavior? What more is there to change?

Also, if rustdoc-theme is set to a dark theme but the system theme is a light theme, can you set use-system-theme to false?

@bors r-

I'd like to address the issues @nasso brought up (thanks for thinking of them!)

I can implement that logic! I suppose that for now I should hard-code what theme is "dark" and what theme is "light"?
At least until #74937 is fixed.

Also, if rustdoc-theme is set to a dark theme but the system theme is a light theme, can you set use-system-theme to false?

This can cause issues:

1. Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.
2. Rustdoc is all using default settings (assuming an empty localStorage). That means it follows the system theme, and writes it to localStorage when switching to it.
3. At night, Rustdoc is using the default dark theme. This is just the default behavior. Now localStorage["rustdoc-theme"] === "dark".
4. The next day, localStorage["rustdoc-theme"] === "dark", but the OS is now in light mode (it's daytime).
5. Rustdoc sets localStorage["rustdoc-use-system-theme"] to false, thinking it was a user of the older Rustdoc version!

One potential fix would be to not save the theme to localStorage when automatically switched to. The downside is that older versions of Rustdoc won't pick up the theme because it's not stored there anymore.

Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.

Seems a bit of an edge case ... but since it leads to rustdoc overriding the user's selection, better to avoid it. Rather than having complicated logic, let's just only implement my first suggestion: If use-system-theme and preferred-dark-theme aren't set, but rustdoc-theme is set to a dark theme, update preferred-dark-theme to that theme.

Someone has their operating system switch automatically between dark mode and light mode according to the time of the day.

Is it an edge case? My phone does this too.

I suppose that for now I should hard-code what theme is "dark" and what theme is "light"?

You can write the canonical list :) There's only three themes, it seems reasonable to have a match on them somewhere.

If use-system-theme and preferred-dark-theme aren't set, but rustdoc-theme is set to a dark theme, update preferred-dark-theme to that theme.

That should be implemented now! I think it's ready now.

Looks great! Thanks so much for working on this :)

@bors r=jyn514,guillaumegomez

📌 Commit 59f9cf2 has been approved by jyn514,guillaumegomez

I wonder if we can have a css fallback rather than relying on javascript? Some users may have disabled javascript.

I wonder if we can have a css fallback rather than relying on javascript? Some users may have disabled javascript.

I thought about this already and I think I may implement a CSS fallback in a future PR. I already have a few ideas about how it could work but I'd prefer to discuss about it first :)

⌛ Testing commit 59f9cf2 with merge 6999ff3...

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514,guillaumegomez
Pushing 6999ff3 to master...